[Flang][OpenMP] Add semantic checks for order clause #115281

Thirumalai-Shaktivel · 2024-11-07T09:05:32Z

Fix:

Move the order clause to allowedOnceClauses list in the OMP_DistributeParallelDoSimd and OMP_TargetParallelDoSimd definitions

OpenMP 5.2:
10.3 Order clause restrictions

If a threadprivate variable is referenced inside a region that corresponds to a construct with an order clause that specifies concurrent, the behavior is unspecified.
A region that corresponds to a construct with an order clause that specifies concurrent may not contain OpenMP runtime API calls.

OpenMP 5.1:
2.11.3 order Clause restriction:

At most one order clause may appear on a construct.

llvmbot · 2024-11-07T09:05:54Z

@llvm/pr-subscribers-flang-openmp

Author: Thirumalai Shaktivel (Thirumalai-Shaktivel)

Changes

Fix:

Move the order clause to allowedOnceClauses list in the OMP_DistributeParallelDoSimd and OMP_TargetParallelDoSimd definitions

OpenMP 5.2:
10.3 Order clause restrictions

A region that corresponds to a construct with an order clause that specifies concurrent may not contain calls to procedures that contain OpenMP directives.
A region that corresponds to a construct with an order clause that specifies concurrent may not contain OpenMP runtime API calls.

OpenMP 5.1:
2.11.3 order Clause restriction:

At most one order clause may appear on a construct.

Full diff: https://github.com/llvm/llvm-project/pull/115281.diff

4 Files Affected:

(modified) flang/lib/Semantics/check-omp-structure.cpp (+40)
(modified) flang/lib/Semantics/check-omp-structure.h (+3)
(added) flang/test/Semantics/OpenMP/order-clause02.f90 (+64)
(modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+6-2)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 014604627f2cd1..cb87ec860d7028 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -598,6 +598,46 @@ void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
     CheckDistLinear(x);
   }
 }
+
+// OpenMP 5.2: 10.3 Order clause restrictions
+void OmpStructureChecker::Enter(const parser::ProcedureDesignator &x) {
+  if (!dirContext_.empty() &&
+      (llvm::omp::allDoSet | llvm::omp::allSimdSet |
+          llvm::omp::allDistributeSet)
+          .test(GetContext().directive)) {
+    if (FindClause(llvm::omp::Clause::OMPC_order)) {
+      const auto &name{std::get<parser::Name>(x.u)};
+      if (std::get<parser::OmpOrderClause::Type>(orderClause.v.t) ==
+              parser::OmpOrderClause::Type::Concurrent &&
+          llvm::StringRef(name.ToString()).starts_with_insensitive("omp_")) {
+        context_.Say(name.source,
+            "The OpenMP runtime API calls are not allowed in "
+            "the `order(concurrent)` clause region"_err_en_US);
+      }
+    }
+  }
+}
+
+// OpenMP 5.2: 10.3 Order clause restrictions
+void OmpStructureChecker::Enter(const parser::Designator &x) {
+  if (!dirContext_.empty() &&
+      (llvm::omp::allDoSet | llvm::omp::allSimdSet |
+          llvm::omp::allDistributeSet)
+          .test(GetContext().directive)) {
+    if (const auto *clause{FindClause(llvm::omp::Clause::OMPC_order)}) {
+      const auto &orderClause{std::get<parser::OmpClause::Order>(clause->u)};
+      const auto &name{parser::Unwrap<parser::Name>(x.u)};
+      if (std::get<parser::OmpOrderClause::Type>(orderClause.v.t) ==
+              parser::OmpOrderClause::Type::Concurrent &&
+          name->symbol->test(Symbol::Flag::OmpThreadprivate)) {
+        context_.Say(name->source,
+            "A THREADPRIVATE variable cannot appear in an `order(concurrent)` "
+            "clause region, the behavior is unspecified"_err_en_US);
+      }
+    }
+  }
+}
+
 const parser::Name OmpStructureChecker::GetLoopIndex(
     const parser::DoConstruct *x) {
   using Bounds = parser::LoopControl::Bounds;
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index d9236be8bced4f..690805f9a9d66b 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -131,6 +131,9 @@ class OmpStructureChecker
   void Enter(const parser::OmpAtomicCapture &);
   void Leave(const parser::OmpAtomic &);
 
+  void Enter(const parser::ProcedureDesignator &);
+  void Enter(const parser::Designator &);
+
 #define GEN_FLANG_CLAUSE_CHECK_ENTER
 #include "llvm/Frontend/OpenMP/OMP.inc"
 
diff --git a/flang/test/Semantics/OpenMP/order-clause02.f90 b/flang/test/Semantics/OpenMP/order-clause02.f90
new file mode 100644
index 00000000000000..c39fb27d657fcc
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/order-clause02.f90
@@ -0,0 +1,64 @@
+! REQUIRES: openmp_runtime
+! RUN: %python %S/../test_errors.py %s %flang_fc1 %openmp_flags -fopenmp-version=50
+! OpenMP Version 5.2
+! Various checks for the order clause
+! 10.3 `order` Clause
+
+! Case 1
+subroutine omp_order_runtime_api_call_01()
+    use omp_lib
+    integer :: i
+    !$omp do order(concurrent)
+    do i = 1, 5
+        !ERROR: The OpenMP runtime API calls are not allowed in the `order(concurrent)` clause region
+        print*, omp_get_thread_num()
+    end do
+    !$omp end do
+end subroutine omp_order_runtime_api_call_01
+
+subroutine omp_order_runtime_api_call_02()
+    use omp_lib
+    integer :: i, num_threads
+    !$omp do order(concurrent)
+    do i = 1, 5
+        !ERROR: The OpenMP runtime API calls are not allowed in the `order(concurrent)` clause region
+        call omp_set_num_threads(num_threads)
+    end do
+    !$omp end do
+end subroutine omp_order_runtime_api_call_02
+
+! Case 2
+subroutine test_order_threadprivate()
+    integer :: i, j = 1, x
+    !$omp threadprivate(j)
+    !$omp parallel do order(concurrent)
+    do i = 1, 5
+        !ERROR: A THREADPRIVATE variable cannot appear in an `order(concurrent)` clause region, the behavior is unspecified
+        j = x + 1
+    end do
+    !$omp end parallel do
+end subroutine
+
+! Case 3
+subroutine omp_order_duplicate_01()
+    implicit none
+    integer :: i, j
+    !ERROR: At most one ORDER clause can appear on the TARGET PARALLEL DO SIMD directive
+    !$OMP target parallel do simd ORDER(concurrent) ORDER(concurrent)
+    do i = 1, 5
+        j = j + 1
+    end do
+    !$omp end target parallel do simd
+end subroutine
+
+subroutine omp_order_duplicate_02()
+    integer :: i, j
+    !$omp teams
+    !ERROR: At most one ORDER clause can appear on the DISTRIBUTE PARALLEL DO SIMD directive
+    !$omp distribute parallel do simd order(concurrent) order(concurrent)
+    do i = 1, 5
+        j = j + 1
+    end do
+    !$omp end distribute parallel do simd
+    !$omp end teams
+end subroutine
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index 36834939d9b451..29f6e65f43b38a 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -1235,7 +1235,6 @@ def OMP_DistributeParallelDoSimd : Directive<"distribute parallel do simd"> {
     VersionedClause<OMPC_Linear>,
     VersionedClause<OMPC_NonTemporal>,
     VersionedClause<OMPC_NumThreads>,
-    VersionedClause<OMPC_Order, 50>,
     VersionedClause<OMPC_Private>,
     VersionedClause<OMPC_ProcBind>,
     VersionedClause<OMPC_Reduction>,
@@ -1244,6 +1243,9 @@ def OMP_DistributeParallelDoSimd : Directive<"distribute parallel do simd"> {
     VersionedClause<OMPC_Shared>,
     VersionedClause<OMPC_SimdLen>,
   ];
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_Order, 50>,
+  ];
   let leafConstructs = [OMP_Distribute, OMP_Parallel, OMP_Do, OMP_Simd];
   let category = CA_Executable;
 }
@@ -1908,7 +1910,6 @@ def OMP_TargetParallelDoSimd : Directive<"target parallel do simd"> {
     VersionedClause<OMPC_NonTemporal>,
     VersionedClause<OMPC_NoWait>,
     VersionedClause<OMPC_NumThreads>,
-    VersionedClause<OMPC_Order, 50>,
     VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_Private>,
     VersionedClause<OMPC_ProcBind>,
@@ -1919,6 +1920,9 @@ def OMP_TargetParallelDoSimd : Directive<"target parallel do simd"> {
     VersionedClause<OMPC_SimdLen>,
     VersionedClause<OMPC_UsesAllocators>,
   ];
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_Order, 50>
+  ];
   let leafConstructs = [OMP_Target, OMP_Parallel, OMP_Do, OMP_Simd];
   let category = CA_Executable;
 }

llvmbot · 2024-11-07T09:05:54Z

@llvm/pr-subscribers-flang-semantics

Author: Thirumalai Shaktivel (Thirumalai-Shaktivel)

Changes

Fix:

Move the order clause to allowedOnceClauses list in the OMP_DistributeParallelDoSimd and OMP_TargetParallelDoSimd definitions

OpenMP 5.2:
10.3 Order clause restrictions

A region that corresponds to a construct with an order clause that specifies concurrent may not contain calls to procedures that contain OpenMP directives.
A region that corresponds to a construct with an order clause that specifies concurrent may not contain OpenMP runtime API calls.

OpenMP 5.1:
2.11.3 order Clause restriction:

At most one order clause may appear on a construct.

Full diff: https://github.com/llvm/llvm-project/pull/115281.diff

4 Files Affected:

(modified) flang/lib/Semantics/check-omp-structure.cpp (+40)
(modified) flang/lib/Semantics/check-omp-structure.h (+3)
(added) flang/test/Semantics/OpenMP/order-clause02.f90 (+64)
(modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+6-2)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 014604627f2cd1..cb87ec860d7028 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -598,6 +598,46 @@ void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
     CheckDistLinear(x);
   }
 }
+
+// OpenMP 5.2: 10.3 Order clause restrictions
+void OmpStructureChecker::Enter(const parser::ProcedureDesignator &x) {
+  if (!dirContext_.empty() &&
+      (llvm::omp::allDoSet | llvm::omp::allSimdSet |
+          llvm::omp::allDistributeSet)
+          .test(GetContext().directive)) {
+    if (FindClause(llvm::omp::Clause::OMPC_order)) {
+      const auto &name{std::get<parser::Name>(x.u)};
+      if (std::get<parser::OmpOrderClause::Type>(orderClause.v.t) ==
+              parser::OmpOrderClause::Type::Concurrent &&
+          llvm::StringRef(name.ToString()).starts_with_insensitive("omp_")) {
+        context_.Say(name.source,
+            "The OpenMP runtime API calls are not allowed in "
+            "the `order(concurrent)` clause region"_err_en_US);
+      }
+    }
+  }
+}
+
+// OpenMP 5.2: 10.3 Order clause restrictions
+void OmpStructureChecker::Enter(const parser::Designator &x) {
+  if (!dirContext_.empty() &&
+      (llvm::omp::allDoSet | llvm::omp::allSimdSet |
+          llvm::omp::allDistributeSet)
+          .test(GetContext().directive)) {
+    if (const auto *clause{FindClause(llvm::omp::Clause::OMPC_order)}) {
+      const auto &orderClause{std::get<parser::OmpClause::Order>(clause->u)};
+      const auto &name{parser::Unwrap<parser::Name>(x.u)};
+      if (std::get<parser::OmpOrderClause::Type>(orderClause.v.t) ==
+              parser::OmpOrderClause::Type::Concurrent &&
+          name->symbol->test(Symbol::Flag::OmpThreadprivate)) {
+        context_.Say(name->source,
+            "A THREADPRIVATE variable cannot appear in an `order(concurrent)` "
+            "clause region, the behavior is unspecified"_err_en_US);
+      }
+    }
+  }
+}
+
 const parser::Name OmpStructureChecker::GetLoopIndex(
     const parser::DoConstruct *x) {
   using Bounds = parser::LoopControl::Bounds;
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index d9236be8bced4f..690805f9a9d66b 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -131,6 +131,9 @@ class OmpStructureChecker
   void Enter(const parser::OmpAtomicCapture &);
   void Leave(const parser::OmpAtomic &);
 
+  void Enter(const parser::ProcedureDesignator &);
+  void Enter(const parser::Designator &);
+
 #define GEN_FLANG_CLAUSE_CHECK_ENTER
 #include "llvm/Frontend/OpenMP/OMP.inc"
 
diff --git a/flang/test/Semantics/OpenMP/order-clause02.f90 b/flang/test/Semantics/OpenMP/order-clause02.f90
new file mode 100644
index 00000000000000..c39fb27d657fcc
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/order-clause02.f90
@@ -0,0 +1,64 @@
+! REQUIRES: openmp_runtime
+! RUN: %python %S/../test_errors.py %s %flang_fc1 %openmp_flags -fopenmp-version=50
+! OpenMP Version 5.2
+! Various checks for the order clause
+! 10.3 `order` Clause
+
+! Case 1
+subroutine omp_order_runtime_api_call_01()
+    use omp_lib
+    integer :: i
+    !$omp do order(concurrent)
+    do i = 1, 5
+        !ERROR: The OpenMP runtime API calls are not allowed in the `order(concurrent)` clause region
+        print*, omp_get_thread_num()
+    end do
+    !$omp end do
+end subroutine omp_order_runtime_api_call_01
+
+subroutine omp_order_runtime_api_call_02()
+    use omp_lib
+    integer :: i, num_threads
+    !$omp do order(concurrent)
+    do i = 1, 5
+        !ERROR: The OpenMP runtime API calls are not allowed in the `order(concurrent)` clause region
+        call omp_set_num_threads(num_threads)
+    end do
+    !$omp end do
+end subroutine omp_order_runtime_api_call_02
+
+! Case 2
+subroutine test_order_threadprivate()
+    integer :: i, j = 1, x
+    !$omp threadprivate(j)
+    !$omp parallel do order(concurrent)
+    do i = 1, 5
+        !ERROR: A THREADPRIVATE variable cannot appear in an `order(concurrent)` clause region, the behavior is unspecified
+        j = x + 1
+    end do
+    !$omp end parallel do
+end subroutine
+
+! Case 3
+subroutine omp_order_duplicate_01()
+    implicit none
+    integer :: i, j
+    !ERROR: At most one ORDER clause can appear on the TARGET PARALLEL DO SIMD directive
+    !$OMP target parallel do simd ORDER(concurrent) ORDER(concurrent)
+    do i = 1, 5
+        j = j + 1
+    end do
+    !$omp end target parallel do simd
+end subroutine
+
+subroutine omp_order_duplicate_02()
+    integer :: i, j
+    !$omp teams
+    !ERROR: At most one ORDER clause can appear on the DISTRIBUTE PARALLEL DO SIMD directive
+    !$omp distribute parallel do simd order(concurrent) order(concurrent)
+    do i = 1, 5
+        j = j + 1
+    end do
+    !$omp end distribute parallel do simd
+    !$omp end teams
+end subroutine
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index 36834939d9b451..29f6e65f43b38a 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -1235,7 +1235,6 @@ def OMP_DistributeParallelDoSimd : Directive<"distribute parallel do simd"> {
     VersionedClause<OMPC_Linear>,
     VersionedClause<OMPC_NonTemporal>,
     VersionedClause<OMPC_NumThreads>,
-    VersionedClause<OMPC_Order, 50>,
     VersionedClause<OMPC_Private>,
     VersionedClause<OMPC_ProcBind>,
     VersionedClause<OMPC_Reduction>,
@@ -1244,6 +1243,9 @@ def OMP_DistributeParallelDoSimd : Directive<"distribute parallel do simd"> {
     VersionedClause<OMPC_Shared>,
     VersionedClause<OMPC_SimdLen>,
   ];
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_Order, 50>,
+  ];
   let leafConstructs = [OMP_Distribute, OMP_Parallel, OMP_Do, OMP_Simd];
   let category = CA_Executable;
 }
@@ -1908,7 +1910,6 @@ def OMP_TargetParallelDoSimd : Directive<"target parallel do simd"> {
     VersionedClause<OMPC_NonTemporal>,
     VersionedClause<OMPC_NoWait>,
     VersionedClause<OMPC_NumThreads>,
-    VersionedClause<OMPC_Order, 50>,
     VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_Private>,
     VersionedClause<OMPC_ProcBind>,
@@ -1919,6 +1920,9 @@ def OMP_TargetParallelDoSimd : Directive<"target parallel do simd"> {
     VersionedClause<OMPC_SimdLen>,
     VersionedClause<OMPC_UsesAllocators>,
   ];
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_Order, 50>
+  ];
   let leafConstructs = [OMP_Target, OMP_Parallel, OMP_Do, OMP_Simd];
   let category = CA_Executable;
 }

Fix: - Move the order clause to allowedOnceClauses list in the `OMP_DistributeParallelDoSimd` and `OMP_TargetParallelDoSimd` definitions OpenMP 5.2: 10.3 Order clause restrictions - A region that corresponds to a construct with an order clause that specifies concurrent may not contain calls to procedures that contain OpenMP directives. - A region that corresponds to a construct with an order clause that specifies concurrent may not contain OpenMP runtime API calls. OpenMP 5.1: 2.11.3 order Clause restriction: - At most one order clause may appear on a construct.

kiranchandramohan · 2024-11-07T10:24:54Z

Regions in OpenMP are generally dynamic (and not static). Will the checks work across function/subroutine calls?

subroutine omp_order_runtime_api_call_01()
    integer :: i
    !$omp do order(concurrent)
    do i = 1, 5
      call myf()        
    end do
    !$omp end do
contains
  subroutine myf
    print*, omp_get_thread_num()
  end subroutine
end subroutine omp_order_runtime_api_call_01

Thirumalai-Shaktivel · 2024-11-07T13:47:25Z

Yes, regarding that, I was wondering, how to get the details of the region?
There is another restriction in Order clause which say:

A region that corresponds to a construct with an order clause that specifies concurrent may
not contain calls to procedures that contain OpenMP directives.

If we get the answer for this, your example would have the similar fix.

kiranchandramohan · 2024-11-07T18:06:30Z

Yes, regarding that, I was wondering, how to get the details of the region? There is another restriction in Order clause which say:
A region that corresponds to a construct with an order clause that specifies concurrent may
not contain calls to procedures that contain OpenMP directives.
If we get the answer for this, your example would have the similar fix.

Generally, we do not support these checks.

Thirumalai-Shaktivel · 2024-11-08T03:05:27Z

Okay, I got it.

Shall we keep the following restrictions, then?

OpenMP 5.2: 10.3 Order clause restrictions

- If a threadprivate variable is referenced inside a region that corresponds to a construct with an order clause that specifies concurrent, the behavior is unspecified.

OpenMP 5.1:
2.11.3 order Clause restriction:

- At most one order clause may appear on a construct.

Thirumalai-Shaktivel · 2024-11-08T03:54:54Z

It says "If a threadprivate variable is referenced inside a region ...".

This might be not possible as well.
I'm closing this PR.

Thirumalai-Shaktivel · 2024-11-08T03:55:10Z

Thanks for the review, @kiranchandramohan

Thirumalai-Shaktivel added flang:openmp flang:semantics labels Nov 7, 2024

Thirumalai-Shaktivel requested review from NimishMishra, kaviya2510, kiranchandramohan, kiranktp, kparzysz and tblah November 7, 2024 09:05

llvmbot added flang Flang issues not falling into any other category clang:openmp OpenMP related changes to Clang labels Nov 7, 2024

Thirumalai-Shaktivel force-pushed the llvm/order_clause_01 branch from 5791620 to d184299 Compare November 7, 2024 10:23

Thirumalai-Shaktivel force-pushed the llvm/order_clause_01 branch from d184299 to 59ff7bb Compare November 7, 2024 10:25

Thirumalai-Shaktivel closed this Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Flang][OpenMP] Add semantic checks for order clause #115281

[Flang][OpenMP] Add semantic checks for order clause #115281

Uh oh!

Thirumalai-Shaktivel commented Nov 7, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 7, 2024

Uh oh!

llvmbot commented Nov 7, 2024

Uh oh!

kiranchandramohan commented Nov 7, 2024

Uh oh!

Thirumalai-Shaktivel commented Nov 7, 2024

Uh oh!

kiranchandramohan commented Nov 7, 2024

Uh oh!

Thirumalai-Shaktivel commented Nov 8, 2024 •

edited

Loading

Uh oh!

Thirumalai-Shaktivel commented Nov 8, 2024

Uh oh!

Thirumalai-Shaktivel commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Flang][OpenMP] Add semantic checks for order clause #115281

[Flang][OpenMP] Add semantic checks for order clause #115281

Uh oh!

Conversation

Thirumalai-Shaktivel commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 7, 2024

Uh oh!

llvmbot commented Nov 7, 2024

Uh oh!

kiranchandramohan commented Nov 7, 2024

Uh oh!

Thirumalai-Shaktivel commented Nov 7, 2024

Uh oh!

kiranchandramohan commented Nov 7, 2024

Uh oh!

Thirumalai-Shaktivel commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Nov 8, 2024

Uh oh!

Thirumalai-Shaktivel commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Thirumalai-Shaktivel commented Nov 7, 2024 •

edited

Loading

Thirumalai-Shaktivel commented Nov 8, 2024 •

edited

Loading